fix(rc): Set explicit English locale for date parsing#1199
fix(rc): Set explicit English locale for date parsing#1199jungsehui wants to merge 1 commit intofirebase:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug where date parsing in Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a date parsing issue that occurs in non-English locales by explicitly setting Locale.ENGLISH when creating SimpleDateFormat instances. This resolves the immediate bug. I've also added a suggestion to modernize the code by migrating from the legacy SimpleDateFormat to the thread-safe java.time.DateTimeFormatter API, which would improve code quality and performance.
| // sample output date string: 2020-11-12T22:12:02.000000000Z | ||
| checkArgument(millis >= 0, "Milliseconds duration must not be negative"); | ||
| SimpleDateFormat dateFormat = new SimpleDateFormat(ZULU_DATE_PATTERN); | ||
| SimpleDateFormat dateFormat = new SimpleDateFormat(ZULU_DATE_PATTERN, Locale.ENGLISH); |
There was a problem hiding this comment.
While adding Locale.ENGLISH fixes the immediate bug, SimpleDateFormat is an outdated and mutable API. Since this project supports Java 8+, it's highly recommended to migrate to the modern java.time API for date and time operations.
java.time.format.DateTimeFormatter is immutable and thread-safe. This means you can define formatters as static final constants and reuse them without needing ThreadLocal or synchronization. This makes the code cleaner, safer, and more performant.
For example, this method could be rewritten as:
// At class level:
private static final DateTimeFormatter ZULU_DATE_FORMATTER =
DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSSSSSSSS'Z'")
.withZone(ZoneOffset.UTC);
// In convertToUtcZuluFormat method:
static String convertToUtcZuluFormat(long millis) {
checkArgument(millis >= 0, "Milliseconds duration must not be negative");
return ZULU_DATE_FORMATTER.format(Instant.ofEpochMilli(millis));
}The original pattern yyyy-MM-dd'T'HH:mm:ss.SSS000000'Z' is correctly implemented with java.time's yyyy-MM-dd'T'HH:mm:ss.SSSSSSSSS'Z'. A similar refactoring can be applied to the other date utility methods in this class. For UTC_DATE_PATTERN, you can even use the pre-defined DateTimeFormatter.RFC_1123_DATE_TIME.
|
Thanks for the suggestion, @gemini-code-assist! While migrating to My intention was to keep the bug fix as minimal and safe as possible without changing the underlying API or expanding the scope of the PR (especially regarding the manual fractional seconds handling currently implemented in the methods). I'm happy to update this to |
|
Thanks for explaining your reasoning, While migrating to |
1 participant
Fixes #1196
Discussion
SimpleDateFormatfails or misbehaves in non-English locales (e.g., Korean).Testing
mvn test).mvn validate).-Duser.language=ko -Duser.country=KRto ensure theRemoteConfigUtilnow safely handles RFC 1123 and Zulu dates regardless of the host OS locale.API Changes
RemoteConfigUtil.